Conversation
There was a problem hiding this comment.
Hi @iamGavinJ
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
📝 WalkthroughWalkthroughIntroduces Samba addon v12.6.1: adds macOS mDNS discovery (mdnsd) and mDNS templates, makes NetBIOS optional with conditional nmbd startup, adds configurable SMB/NetBIOS ports, relocates share paths under /smbshare, renames and refactors Samba templates, updates config schema and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Init as Init Script
participant Bashio as bashio
participant JQ as jq
participant Tempio as tempio
participant mdnsd as mdnsd
Init->>Bashio: read hostname & network ports
Bashio-->>Init: hostname, ports
Init->>JQ: merge ports + samba version -> options.json
JQ-->>Init: options.json
Init->>Tempio: render smb.service.gtpl -> smb advertisement
Tempio-->>Init: smb.service file
Init->>Tempio: render adisk.service.gtpl -> adisk advertisement
Tempio-->>Init: adisk.service file
Init->>mdnsd: launch mdnsd with generated advertisements
mdnsd-->>Init: register mDNS services
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@samba/Dockerfile`:
- Around line 9-12: The Dockerfile currently installs mdnsd@testing unpinned,
which risks nondeterministic builds; update the apk add invocation to install
mdnsd with an explicit package version (e.g., mdnsd=<pkgver>-r<release> or
mdnsd=<version>@testing) instead of mdnsd@testing. Determine the exact version
to pin by querying the apk repository (apk search/info or apk policy for mdnsd)
in your build environment, then replace the mdnsd@testing token in the apk add
line with the pinned package string (keeping the repository tag if required) so
the Dockerfile’s apk add installs a specific mdnsd release for reproducible
builds.
In `@samba/rootfs/etc/s6-overlay/s6-rc.d/mdnsd/finish`:
- Line 5: The finish script currently runs the command "killall mdnsd" which
returns a non-zero exit when no mdnsd process exists; update the finish script
to make teardown tolerant by invoking killall in a way that ignores "no process"
failures (e.g., use the killall quiet flag or otherwise swallow the exit code)
so the script does not error when mdnsd is already stopped; locate the "killall
mdnsd" invocation in the finish script and change it to a tolerant form (e.g.,
quiet/silent option or append a no-fail fallback) so the overall finish step
succeeds even if mdnsd is absent.
In `@samba/rootfs/etc/s6-overlay/s6-rc.d/mdnsd/run`:
- Around line 22-25: In the s6 run script's jq block that builds the JSON
(.ports and .sambaversion), fix the invalid cut flag by replacing the `cut -F 2`
usage with the correct `cut -f 2` when extracting the second field from
`smbstatus --version`; update the command referenced in the jq string (the
`$(smbstatus --version | cut -f 2)` fragment) so the version extraction succeeds
and the JSON payload written to "${_TMP}" is valid.
- Line 7: Replace the insecure dry-run mktemp call that assigns declare -r
_TMP="$(mktemp -u -t XXXXXX).json" with an atomic creation: call mktemp (without
-u) to create the temp file and assign its path to _TMP (preserving the .json
suffix if desired), and add a trap 'EXIT' handler to remove "$_TMP" on script
exit; update any subsequent uses that write to or read from _TMP to rely on the
created file.
In `@samba/rootfs/usr/share/tempio/smb.service.gtpl`:
- Line 4: The template always falls back to 445 because the default is
hard-coded; change the port expression so it prefers .ports["445/tcp"] then
.ports["139/tcp"] before defaulting to 445. Locate the line using the index
.ports "445/tcp" | default 445 and replace its default with a nested lookup
(e.g. use index .ports "445/tcp" | default (index .ports "139/tcp" | default
445)) so configured 139/tcp is honored when 445/tcp is empty.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
samba/rootfs/etc/s6-overlay/s6-rc.d/.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (16)
samba/CHANGELOG.mdsamba/Dockerfilesamba/config.yamlsamba/rootfs/etc/s6-overlay/s6-rc.d/init-smbd/runsamba/rootfs/etc/s6-overlay/s6-rc.d/mdnsd/dependencies.d/smbdsamba/rootfs/etc/s6-overlay/s6-rc.d/mdnsd/finishsamba/rootfs/etc/s6-overlay/s6-rc.d/mdnsd/runsamba/rootfs/etc/s6-overlay/s6-rc.d/mdnsd/typesamba/rootfs/etc/s6-overlay/s6-rc.d/mdnsd/upsamba/rootfs/etc/s6-overlay/s6-rc.d/nmbd/runsamba/rootfs/etc/s6-overlay/s6-rc.d/smbd/runsamba/rootfs/etc/s6-overlay/s6-rc.d/user/contents.d/mdnsdsamba/rootfs/usr/share/tempio/adisk.service.gtplsamba/rootfs/usr/share/tempio/smb.conf.gtplsamba/rootfs/usr/share/tempio/smb.service.gtplsamba/translations/en.yaml
samba/Dockerfile
Outdated
| && mkdir -p /var/lib/samba \ | ||
| echo '@testing https://dl-cdn.alpinelinux.org/alpine/edge/testing' >> \ | ||
| /etc/apk/repositories && \ | ||
| apk add --no-cache samba mdnsd@testing \ |
There was a problem hiding this comment.
Hm, I see this daemon only got introduced in current testing.
Maybe we should add a apps configuration key which allows to register a service, then Home Assistant Supervisor could use the OS level mDNS announcer systemd-resolved, via RegisterService call on the org.freedesktop.resolve1 service 🤔 .
There was a problem hiding this comment.
Hi @agners I completely agree, that's a much cleaner solution, expecially since there really should only be one dns-sd/mDNS service servicing requests on any individual interface.
In the future that sort of call could also be exposed via basio.
I'd be happy to refactor again once that infrastructure is available.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@samba/rootfs/usr/share/tempio/smb.conf.gtpl`:
- Around line 1-12: Replace the service-discovery style name used in the
"additional dns hostnames" template with a proper FQDN; specifically update the
line that currently reads additional dns hostnames = {{ env "HOSTNAME"
}}._smb._tcp.local to use a hostname FQDN such as {{ env "HOSTNAME" }}.local (or
another valid domain), i.e. adjust the template expression for the "additional
dns hostnames" key to emit a standard DNS hostname rather than the DNS‑SD
service name; keep mdns/._smb._tcp service announcements handled by the
mdnsd/.service templates instead.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (18)
.gitignoresamba/CHANGELOG.mdsamba/Dockerfilesamba/config.yamlsamba/rootfs/etc/s6-overlay/s6-rc.d/init-smbd/runsamba/rootfs/etc/s6-overlay/s6-rc.d/mdnsd/dependencies.d/smbdsamba/rootfs/etc/s6-overlay/s6-rc.d/mdnsd/runsamba/rootfs/etc/s6-overlay/s6-rc.d/mdnsd/typesamba/rootfs/etc/s6-overlay/s6-rc.d/mdnsd/upsamba/rootfs/etc/s6-overlay/s6-rc.d/nmbd/runsamba/rootfs/etc/s6-overlay/s6-rc.d/smbd/runsamba/rootfs/etc/s6-overlay/s6-rc.d/user/contents.d/mdnsdsamba/rootfs/usr/share/tempio/adisk.service.gtplsamba/rootfs/usr/share/tempio/smb.conf.gtplsamba/rootfs/usr/share/tempio/smb.conf.inc.gtplsamba/rootfs/usr/share/tempio/smb.gtplsamba/rootfs/usr/share/tempio/smb.service.gtplsamba/translations/en.yaml
💤 Files with no reviewable changes (1)
- samba/rootfs/usr/share/tempio/smb.gtpl
✅ Files skipped from review due to trivial changes (2)
- samba/CHANGELOG.md
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (5)
- samba/rootfs/etc/s6-overlay/s6-rc.d/mdnsd/type
- samba/rootfs/etc/s6-overlay/s6-rc.d/init-smbd/run
- samba/rootfs/etc/s6-overlay/s6-rc.d/mdnsd/up
- samba/rootfs/etc/s6-overlay/s6-rc.d/mdnsd/run
- samba/rootfs/usr/share/tempio/adisk.service.gtpl
There was a problem hiding this comment.
🧹 Nitpick comments (3)
samba/DOCS.md (3)
134-134: Use sentence-style capitalization in heading.Change
## Network Portsto## Network ports.As per coding guidelines, "Use sentence-style capitalization also in headings."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@samba/DOCS.md` at line 134, The heading "## Network Ports" should use sentence-style capitalization; update the heading text in DOCS.md from "## Network Ports" to "## Network ports" so it matches the project's heading capitalization guidelines.
137-137: Format UI string as bold, not quoted text.Use Connect to server... instead of
"Connect to server...".As per coding guidelines, "Use bold to mark UI strings" and 'If "" are used to mark UI strings, replace them by bold.'
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@samba/DOCS.md` at line 137, Replace the quoted UI string "Connect to server..." with a bolded UI string **Connect to server...** in the DOCS.md text so it follows the project guideline of using bold for UI strings; ensure you remove the double quotes and wrap the phrase with double asterisks exactly as used elsewhere in docs.
119-121: Tighten wording for concise, authoritative tone.
very old legacyis redundant; simplify tolegacyand rewrite the NOTE sentence more directly.As per coding guidelines, "Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@samba/DOCS.md` around lines 119 - 121, Replace the phrase "very old legacy" with "legacy" in the NetBIOS paragraph and rewrite the NOTE sentence to a direct, concise statement (e.g., "This setting is enabled by default for compatibility; disable it on modern installations.") so the NetBIOS section reads authoritatively and follows Microsoft Style Guide guidance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@samba/DOCS.md`:
- Line 134: The heading "## Network Ports" should use sentence-style
capitalization; update the heading text in DOCS.md from "## Network Ports" to
"## Network ports" so it matches the project's heading capitalization
guidelines.
- Line 137: Replace the quoted UI string "Connect to server..." with a bolded UI
string **Connect to server...** in the DOCS.md text so it follows the project
guideline of using bold for UI strings; ensure you remove the double quotes and
wrap the phrase with double asterisks exactly as used elsewhere in docs.
- Around line 119-121: Replace the phrase "very old legacy" with "legacy" in the
NetBIOS paragraph and rewrite the NOTE sentence to a direct, concise statement
(e.g., "This setting is enabled by default for compatibility; disable it on
modern installations.") so the NetBIOS section reads authoritatively and follows
Microsoft Style Guide guidance.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📥 Commits
Reviewing files that changed from the base of the PR and between 2c6ba79 and 9eebe9322ffb80c6b9f7abe2b77ff25b7f74e19c.
📒 Files selected for processing (1)
samba/DOCS.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
samba/DOCS.md (1)
136-140: Rewrite this as explicit task-first instructions.This section is procedural guidance, but the action is not fronted. Rephrase to a direct instruction format (goal first, then location), e.g., “To connect after changing ports, in Finder open Connect to server... and enter
smb://<hostname>:<port>/<sharename>.”As per coding guidelines, "In step-by-step instructions, front the 'goal' in the instructional sentence."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@samba/DOCS.md` around lines 136 - 140, Rewrite the paragraph as a task-first, imperative instruction: start with the goal "To connect after changing ports," then give the action and location — e.g., "To connect after changing ports, in Finder open Connect to server... (⌘ + K) and enter smb://<hostname>:<port>/<sharename>." Ensure the note about Finder's Network browser limitation is included as a brief follow-up sentence (e.g., "Note: Finder's Network location browser cannot access shares on non-default ports"), and keep the protocol example string exactly as shown: smb://<hostname>:<port>/<sharename>.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@samba/DOCS.md`:
- Around line 130-133: The docs incorrectly mark the `allow_hosts` option as
"required" while the example shows a default value; update the heading and
description for `allow_hosts` (the "### Option: `allow_hosts`" section) to
indicate it is optional and document the default (e.g., "optional (default:
...)"/"optional — default: ...") and adjust the text to say "List of
hosts/networks allowed to access the shared folders. If omitted, defaults to
<default_value>." Ensure the label and description consistently reflect it is
not required.
---
Nitpick comments:
In `@samba/DOCS.md`:
- Around line 136-140: Rewrite the paragraph as a task-first, imperative
instruction: start with the goal "To connect after changing ports," then give
the action and location — e.g., "To connect after changing ports, in Finder open
Connect to server... (⌘ + K) and enter smb://<hostname>:<port>/<sharename>."
Ensure the note about Finder's Network browser limitation is included as a brief
follow-up sentence (e.g., "Note: Finder's Network location browser cannot access
shares on non-default ports"), and keep the protocol example string exactly as
shown: smb://<hostname>:<port>/<sharename>.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📥 Commits
Reviewing files that changed from the base of the PR and between 9eebe9322ffb80c6b9f7abe2b77ff25b7f74e19c and 43c64fd.
📒 Files selected for processing (1)
samba/DOCS.md
agners
left a comment
There was a problem hiding this comment.
Generally nice cleanup and modernization.
I think using mdnsd from testing is fine. I'd suggest we make "Announce via mDNS/DNS-SD" an optional boolean flag, which we would default to false at first, so we can test it a before rolling out to everyone. If we don't receive negative feedback, we can default the optional flag to true, essentially rolling it out to all users.
| # shellcheck shell=bash | ||
|
|
||
|
|
||
| if bashio::var.true "$(bashio::addon.host_network)"; then |
There was a problem hiding this comment.
Since host_network is really statically configured, I don't think this is necessary, it will evaluate to true always. Simply run the service.
| # ============================================================================== | ||
| exec nmbd \ | ||
|
|
||
| if ! bashio::config.true 'netbios'; then |
There was a problem hiding this comment.
There is a cleaner way to enable/disable services, have a look at rootfs/etc/s6-overlay/scripts/enable-check.sh in the openthread_border_router app. Don't forget to add the script to S6_STAGE2_HOOK.
| @@ -1,5 +1,5 @@ | |||
| --- | |||
| version: 12.6.0 | |||
| version: 12.6.1 | |||
There was a problem hiding this comment.
I'd say this is at least a new feature, so warrants a 12.7.0.
mdnsdfor macOS discoverabilitysmb.conftemplate for disambiguation with new templatesnmbdstartup logic, to disable NetBIOS/smbshareprefix to avoid potential confusion.config.yamlmap:to new object syntaxSummary by CodeRabbit
New Features
Refactor
Documentation
Chores